Convert report renderer to esmodules (perma-draft PR) #12623
Conversation
|
We can export the individual esmodule files to google3 and devtools and bundle in those build systems just fine. However, there is a minor nuisance regarding getFilenamePrefix, which has shared usage in report code and CLI code (so is implemented in commonjs). To work around that I shimmed the file on rolling to the two destinations. An alternative is to do the "esmodule bundle", which looks like this https://gist.github.com/connorjclark/25082d90e28675378a1330dde5591179 . the getFilename module is transformed from commonjs to fit nicely in the esmodule bundle. We can import a single bundled esmodule to google3/devtools just as easily as doing each indiviudally. thoughts? |
There was a problem hiding this comment.
there is a minor nuisance regarding getFilenamePrefix, which has shared usage in report code and CLI code (so is implemented in commonjs)
Should we move it to util.js to contain the gymnastics to a single mega-util file? Or would that not really help?
An alternative is to do the "esmodule bundle", which looks like this https://gist.github.com/connorjclark/25082d90e28675378a1330dde5591179 . the getFilename module is transformed from commonjs to fit nicely in the esmodule bundle. We can import a single bundled esmodule to google3/devtools just as easily as doing each indiviudally.
I am a big fan of this regardless. I think the contract that every embedder needs to be able to consume every one of our front-end files individually is extraordinarily limiting. Ideally, we have a very specific contract that we must fulfill (and can easily test against), we build a single file in whatever formats / entrypoints that fulfill the contract, and that's the extent of our embedder policy.
|
|
||
| if (document.readyState === 'loading') { | ||
| window.addEventListener('DOMContentLoaded', __initLighthouseReport__); | ||
| } else { |
There was a problem hiding this comment.
this seems new, isn't __LIGHTHOUSE_JSON__ always going to be undefined when this script executes? now that we moved it into the %%LIGHTHOUSE_JAVASCRIPT%% block?
| echo '// @ts-nocheck' > "$OUT_FILE" | ||
| echo '// Auto-generated by lighthouse-core/scripts/copy-util-commonjs.sh' >> "$OUT_FILE" | ||
| echo '// Temporary solution until all our code uses esmodules' >> "$OUT_FILE" | ||
| sed 's/export //g' "$LH_ROOT_DIR"/lighthouse-core/report/html/renderer/common/util.js >> "$OUT_FILE" |
There was a problem hiding this comment.
I'll admit I was confused about this one until I looked at the class, s/export class Util/class Util/?
|
Super awesome work @connorjclark! I'm glad the shim-process worked out alright for us 🎉 |
There's also this to deal with: https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/panels/lighthouse/LighthouseReportRenderer.ts We extend ReportRenderer (trivially; just a nice place to put some code that could go somewhere else in CDT. Refactoring this would be a good standalone CL) but the way we extend initially I suggested we move this code into GH, but there are so many references to CDT code that we'd be asking for implementation issues in the future and would be on the hook for fixing them. (and we'd have to move the special ReportRenderer code into our codebase too probably, because all that to say, at the moment the only clear path I see for CDT is to continue cherrypicking the DOM, ReportRenderer, ReportUIFeature classes like we are. We could still do a central entrypoint for PSI, though. |
I see one :) interface RendererHooks {
onReportRender(el: HTMLElement, lhr: LH.Result): void
onNodeDetailsRender(el: HTMLElement, details: LH.Details.NodeDetails): void
onSourceLocationRender(el: HTMLElement, details: LH.Details.SourceLocation): void
onPrint(reportEl: HTMLElement, abortController: AbortController): void
onSave(lhr: LH.Result, abortController: AbortController): void
}
interface LighthouseReportRenderer {
render(container: HTMLELement, lhr: LH.Result, hooks: RendererHooks): void
} |
|
for linkifying CDT can just act on the result of the render call (so can drop the first three "hooks" in your example), but +1 to the direction you're going. while we're at it, want to move ReportUIFeatures to ReportRenderer? btw the "hooks" could be merged into this idea #12327 |
|
taking a closer look at psi.js, and I'm starting to realize that "one common renderer interface" is a harder problem than "convert to esmodules", and the former doesn't seem to make the latter any simpler. I suggest we do the esmodule migration before trying to rework CDT's manual DOM/ReportRenderer/ReportUIFeatures usage or PSI's ... whatever you wanna call it. |
Ya, definitely. I agree we don't need to jump straight to API rejiggering. Just trying to establish the premise that embedders shouldn't further entrench reliance on specific individual files we have if we can, so +1 to the esmodule bundle approach for now. We can iterate on the hook-style API once @paulirish wraps up his work on cataloging embedder requirements on the PSI side. |
I'm envisioning a future where Lighthouse might remove and re-add elements to the DOM based on its internal state (via global 3P filtering or something), so embedders should get used to the idea of "hey this element is now available if you want to modify it" rather than a single static tree |
|
Closing this now. Going to follow up with a big PR of just moving files (I think all the report code should be in |
ES Modules is eating the (javascript) world, and now we are evaluating if we should feed our lovely report renderer to the beast.
We've had a couple of requests (including from Chrome DevTools team) to move our report renderer to ES Modules: #11628 #10926
We think the upcoming Fraggle Rock report work will benefit from us using ES modules. This is also an opportunity to align on a common rendering interface across all our clients: standalone, DevTools and PSI. We don't do any hacky commonjs stuff in the report renderer code, so the actual conversion is simple. This work is a nice precursor to an all-esmodules future for Lighthouse.
I started to explore the technical side to see how we'd want to do this. Here's what I learned:
My first pass, I was trying to avoid using a bundler at all for the report code. It worked, but required using a inline esmodules hack. It's actually quite interesting (report, code), but a) there's a noticeable delay in rendering and b) bundling the renderer is still going to be a requirement for other clients, so this isn't a valid approach.
OK, so then I brought in Rollup as a dev dependency.
Some big problems to work through:
renderer/util.jsis used in many places outsiderenderer/, which means converting it to esmodules is problematic (since it needs to continue to be consumed from commonjs). We can convert the file to esmodules if we copy the code that other places need into a commonjs file. Or, we can keep the file as commonjs (rollup can still bundle it). The latter would keep our collect-strings i18n script simpler too, otherwise we need to move the renderer strings / add ability to parse esmodules in that script.Importing to DevTools + "Save as HTML" feature. My first pass was importing each esmodule file individually, but it seems better to bundle them to a mega-esmodule file and only roll that to DevTools. Basically, instead of the current
report.js, we doreport.mjs. This should keep the "Save as HTML" feature working the same. Some problems with DevTools integration, more details in linked comment.Importing for Lightrider / PSI. We're pretty sure we can import esmodules to google3. Unclear if we want to do individual files (like current) and build there, or as esmodule bundle (like the proposal for DevTools).cl/377963333 . We can import individual esmodule files and bundle internally.Consolidating renderer entrypoints. For example, DevTools handles rendering itself by grabbing the Dom, ReportRenderer classes... would be nice if we just had a common function for "renderReport" that returned an element. We'd need to support overriding ReportUIFeatures, though (or, shim it for devtools esm bundle, and also move that code into GitHub) (WIP: report: configure features #12327 might help).